Increase delayBetweenIndividualWorkerRestarts #1992
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code ReviewSummaryThis PR changes the AnalysisWhat Changed:
Impact:
Concerns & Questions
RecommendationsBefore merging:
Performance Considerations
Security & Test Coverage
Overall: The change looks safe but needs better documentation. Please update the PR description with context about what issue this solves before merging. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review for PR #1992Thank you for this PR! I have reviewed the changes and have the following feedback: SummaryThis PR makes three main changes:
Positives
Issues and Recommendations1. Missing PR descriptionThe PR template placeholders have not been filled in. Per CLAUDE.md, you need to explain WHY the delay was increased and what test failures occurred. Understanding the root cause would help determine if 1 minute is the right value or if there is a deeper issue. 2. Code change rationaleThe change removes the CI-specific value from 0.01 to 1 minute. Concerns:
Recommendation: Consider whether this should use an environment variable (RENDERER_DELAY_BETWEEN_INDIVIDUAL_WORKER_RESTARTS already exists per configBuilder.ts:167-169), or use a different value like 0.1 or 0.5 minutes as a middle ground 3. Potential issues in CI workflowThe tail -f process runs in the background indefinitely and is never explicitly killed. If log/test.log does not exist after timeout, the script continues silently. Background processes might outlive the CI job or interfere with cleanup. Recommendation: Add cleanup step or use a timeout mechanism 4. Ruby script qualityThe bin/view-synchronized-logs script is well-written. Minor suggestion: Line 48 could handle malformed timestamps more gracefully as Time.parse will raise an exception on invalid input. Good defensive programming on line 95 with rescue for terminal width! 5. Missing checklist itemsPer the PR template you need to add/update tests, update documentation, and update CHANGELOG file if this is a user-visible change. Security and Performance
CHANGELOG ConsiderationPer CLAUDE.md, only add CHANGELOG entries for user-visible changes. Based on the file changed (react_on_rails_pro/spec/dummy/client/node-renderer.js), this appears to be test configuration, so likely NO CHANGELOG needed unless it fixes a user-facing bug. Action Items
Questions for Author
Overall, the log viewing infrastructure is excellent, but I would like to better understand the rationale for the core configuration change before approving. Great work on the debugging tools! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.github/workflows/pro-integration-tests.yml (1)
249-258: Consider improving wait logic and log visibility.Two observations:
- The timeout variable is actually a decrementing counter (not seconds), which may be confusing. The actual timeout is 5 seconds (10 * 0.5s).
- Unlike the node-renderer logging (line 242), this step doesn't use
tee, so the Rails log won't be visible in the CI output—only in the artifact.Consider this diff for clarity and consistency:
- name: Tail Rails test log with timestamps in background run: | cd spec/dummy # Wait for log file to be created - timeout=10 + max_iterations=10 + iteration=0 - while [ ! -f log/test.log ] && [ $timeout -gt 0 ]; do + while [ ! -f log/test.log ] && [ $iteration -lt $max_iterations ]; do sleep 0.5 - timeout=$((timeout - 1)) + iteration=$((iteration + 1)) done - tail -f log/test.log 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]' > tmp/ci-logs/rails-server.log & + tail -f log/test.log 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]' | tee tmp/ci-logs/rails-server.log &bin/view-synchronized-logs (4)
11-32: Clean class structure with minor style issues.The overall design is sound—clear separation of concerns with initialization, parsing, and display logic. However, string literals on lines 23-24 should use double quotes for consistency.
Apply this diff:
def view - rails_entries = parse_log_file(@rails_log_path, 'RAILS') - node_entries = parse_log_file(@node_log_path, 'NODE') + rails_entries = parse_log_file(@rails_log_path, "RAILS") + node_entries = parse_log_file(@node_log_path, "NODE")
72-109: Address style issues and moderate complexity.Several issues to address:
- Line 73: Missing empty line after guard clause
- Line 77: Cyclomatic complexity is 8/7 (slightly over)
- Line 86: Rescue modifier usage can hide errors
- Line 98: Single-line if statement should use modifier form
Apply these fixes:
def matches_filter?(content) return true unless @filter + content.match?(@filter) end def display_synchronized_logs(rails_entries, node_entries) all_entries = (rails_entries + node_entries).sort_by { |e| e[:time] } if all_entries.empty? puts "No entries to display (possibly filtered out)." return end # Get terminal width - terminal_width = `tput cols`.to_i rescue 160 + terminal_width = begin + `tput cols`.to_i + rescue StandardError + 160 + end column_width = [(terminal_width - 5) / 2, 60].max # Print header print_header(column_width) # Display entries last_time = nil all_entries.each do |entry| # Show time gap if requested if @show_time_gaps && last_time gap = entry[:time] - last_time - if gap > @time_window - print_time_gap(gap, column_width) - end + print_time_gap(gap, column_width) if gap > @time_window end print_entry(entry, column_width) last_time = entry[:time] end # Print summary print_summary(rails_entries, node_entries, all_entries) end
111-167: Multiple style issues in display helper methods.Several linting issues need to be addressed:
- Lines 112, 121, 155: Ambiguous operator precedence (wrap in parentheses)
- Lines 132: Use modifier if for single-line body
- Line 133, 155: Prefer string interpolation over concatenation
- Lines 137, 145, 226, 227: Use double-quoted strings
- Line 161: Use guard clause instead of wrapping in conditional
Apply this diff to address the issues:
def print_header(column_width) - separator = "=" * (column_width * 2 + 5) + separator = "=" * ((column_width * 2) + 5) puts separator rails_header = "RAILS SERVER".center(column_width) node_header = "NODE RENDERER".center(column_width) puts "#{rails_header} | #{node_header}" puts separator end def print_time_gap(gap, column_width) - gap_text = format("--- %.1f second gap ---", gap).center(column_width * 2 + 5) + gap_text = format("--- %.1f second gap ---", gap).center((column_width * 2) + 5) puts "\e[90m#{gap_text}\e[0m" # Gray color end def print_entry(entry, column_width) timestamp = entry[:time].strftime("%H:%M:%S.%3N") content = entry[:content].strip source = entry[:source] # Truncate content if too long max_content_length = column_width - 13 # Account for timestamp - if content.length > max_content_length - content = content[0...max_content_length - 3] + "..." - end + content = "#{content[0...max_content_length - 3]}..." if content.length > max_content_length # Color codes - color = source == 'RAILS' ? "\e[34m" : "\e[32m" # Blue for Rails, Green for Node + color = source == "RAILS" ? "\e[34m" : "\e[32m" # Blue for Rails, Green for Node reset = "\e[0m" time_color = "\e[90m" # Gray for timestamp # Format entry with timestamp formatted_entry = "#{time_color}#{timestamp}#{reset} #{color}#{content}#{reset}" # Print in appropriate column - if source == 'RAILS' + if source == "RAILS" left_content = formatted_entry.ljust(column_width + 20) # +20 for ANSI codes puts "#{left_content} |" else left_padding = " " * column_width puts "#{left_padding} | #{formatted_entry}" end end def print_summary(rails_entries, node_entries, all_entries) - puts "\n" + "=" * 80 + puts "\n#{"=" * 80}" puts "Summary:" puts " Rails entries: #{rails_entries.size}" puts " Node entries: #{node_entries.size}" puts " Total entries: #{all_entries.size}" - if all_entries.any? - duration = all_entries.last[:time] - all_entries.first[:time] - puts " Time span: #{format('%.2f', duration)} seconds" - puts " First entry: #{all_entries.first[:time].strftime('%Y-%m-%d %H:%M:%S.%3N')}" - puts " Last entry: #{all_entries.last[:time].strftime('%Y-%m-%d %H:%M:%S.%3N')}" - end + return unless all_entries.any? + + duration = all_entries.last[:time] - all_entries.first[:time] + puts " Time span: #{format('%.2f', duration)} seconds" + puts " First entry: #{all_entries.first[:time].strftime('%Y-%m-%d %H:%M:%S.%3N')}" + puts " Last entry: #{all_entries.last[:time].strftime('%Y-%m-%d %H:%M:%S.%3N')}" end
170-243: Well-designed CLI interface with minor style issues.The command-line interface is well-structured with clear help text, flexible input options (directory or individual files), and proper validation. The only issues are string literal style violations on lines 226-227.
Fix the string literal style:
- rails_log = File.join(logs_dir, 'rails-server.log') - node_log = File.join(logs_dir, 'node-renderer.log') + rails_log = File.join(logs_dir, "rails-server.log") + node_log = File.join(logs_dir, "node-renderer.log")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pro-integration-tests.yml(2 hunks)bin/view-synchronized-logs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint JS and Ruby
bin/view-synchronized-logs
[warning] 1-1: Lint/ScriptPermission: Script file view-synchronized-logs doesn't have execute permission.
[warning] 4-4: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 5-5: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 23-23: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 24-24: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 36-36: Metrics/CyclomaticComplexity: Cyclomatic complexity for parse_log_file is too high. [12/7]
[warning] 36-36: Metrics/PerceivedComplexity: Perceived complexity for parse_log_file is too high. [13/10]
[warning] 52-52: Style/PerlBackrefs: Prefer ::Regexp.last_match(1) over $1.
[warning] 53-53: Style/PerlBackrefs: Prefer ::Regexp.last_match(2) over $2.
[warning] 73-73: Layout/EmptyLineAfterGuardClause: Add empty line after guard clause.
[warning] 77-77: Metrics/CyclomaticComplexity: Cyclomatic complexity for display_synchronized_logs is too high. [8/7]
[warning] 86-86: Style/RescueModifier: Avoid using rescue in its modifier form.
[warning] 98-98: Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
[warning] 112-112: Lint/AmbiguousOperatorPrecedence: Wrap expressions with varying precedence with parentheses to avoid ambiguity.
[warning] 121-121: Lint/AmbiguousOperatorPrecedence: Wrap expressions with varying precedence with parentheses to avoid ambiguity.
[warning] 132-132: IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
[warning] 133-133: Style/StringConcatenation: Prefer string interpolation to string concatenation.
[warning] 137-137: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 145-145: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 155-155: Style/StringConcatenation: Prefer string interpolation to string concatenation.
[warning] 155-155: Lint/AmbiguousOperatorPrecedence: Wrap expressions with varying precedence with parentheses to avoid ambiguity.
[warning] 161-161: Style/GuardClause: Use a guard clause (return unless all_entries.any?) instead of wrapping the code inside a conditional expression.
[warning] 226-226: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 227-227: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (3)
.github/workflows/pro-integration-tests.yml (3)
233-236: LGTM!Clear installation of the timestamp utility with good documentation explaining its purpose for log synchronization.
238-242: LGTM!The timestamped logging implementation correctly captures both stdout and stderr, applies timestamps in the expected format, and preserves log output while writing to file. The background execution is appropriate for this long-running process.
319-324: LGTM!The artifact upload step correctly captures the synchronized logs directory and uses
if: always()to ensure logs are available even when tests fail—critical for debugging.
bin/view-synchronized-logs
Outdated
| #!/usr/bin/env ruby | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'time' | ||
| require 'optparse' | ||
|
|
||
| # Script to view synchronized logs from CI runs side-by-side | ||
| # Usage: bin/view-synchronized-logs path/to/logs/directory | ||
| # bin/view-synchronized-logs --rails rails.log --node node.log | ||
|
|
There was a problem hiding this comment.
Missing execute permission on script file.
The script file doesn't have execute permission, which will prevent it from being run directly. Additionally, there are string literal style inconsistencies (lines 4-5).
Apply these fixes:
chmod +x bin/view-synchronized-logsAnd update the string literals:
-require 'time'
-require 'optparse'
+require "time"
+require "optparse"🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] 1-1: Lint/ScriptPermission: Script file view-synchronized-logs doesn't have execute permission.
[warning] 4-4: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 5-5: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
🤖 Prompt for AI Agents
In bin/view-synchronized-logs around lines 1 to 10, the script is missing the
executable bit and has inconsistent string literal styles on lines 4-5; make the
file executable (chmod +x bin/view-synchronized-logs) and normalize the require
strings to the project style (e.g., change require 'time' and require 'optparse'
to use double quotes if the repo prefers double quotes or convert all to single
quotes to match project linting) so the script can be run directly and string
literals are consistent.
bin/view-synchronized-logs
Outdated
| def parse_log_file(path, source) | ||
| entries = [] | ||
| return entries unless File.exist?(path) | ||
|
|
||
| current_timestamp = nil | ||
| current_lines = [] | ||
|
|
||
| File.readlines(path).each do |line| | ||
| if line =~ TIMESTAMP_REGEX | ||
| # Save previous entry if exists | ||
| if current_timestamp && !current_lines.empty? | ||
| content = current_lines.join | ||
| entries << { time: current_timestamp, content: content, source: source } if matches_filter?(content) | ||
| end | ||
|
|
||
| # Start new entry | ||
| current_timestamp = Time.parse($1) | ||
| current_lines = [$2] | ||
| elsif current_timestamp | ||
| # Continuation of previous entry | ||
| current_lines << line | ||
| end | ||
| end | ||
|
|
||
| # Add final entry | ||
| if current_timestamp && !current_lines.empty? | ||
| content = current_lines.join | ||
| entries << { time: current_timestamp, content: content, source: source } if matches_filter?(content) | ||
| end | ||
|
|
||
| entries | ||
| rescue StandardError => e | ||
| warn "Error parsing #{path}: #{e.message}" | ||
| [] | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reduce cyclomatic complexity and avoid Perl-style backrefs.
The parse_log_file method has high cyclomatic (12/7) and perceived (13/10) complexity. Additionally, lines 52-53 use discouraged Perl-style backrefs ($1, $2) instead of Regexp.last_match.
Consider extracting helper methods to reduce complexity:
def parse_log_file(path, source)
entries = []
return entries unless File.exist?(path)
current_timestamp = nil
current_lines = []
File.readlines(path).each do |line|
if line =~ TIMESTAMP_REGEX
- # Save previous entry if exists
- if current_timestamp && !current_lines.empty?
- content = current_lines.join
- entries << { time: current_timestamp, content: content, source: source } if matches_filter?(content)
- end
+ save_entry(entries, current_timestamp, current_lines, source) if current_timestamp
# Start new entry
- current_timestamp = Time.parse($1)
- current_lines = [$2]
+ current_timestamp = Time.parse(Regexp.last_match(1))
+ current_lines = [Regexp.last_match(2)]
elsif current_timestamp
# Continuation of previous entry
current_lines << line
end
end
- # Add final entry
- if current_timestamp && !current_lines.empty?
- content = current_lines.join
- entries << { time: current_timestamp, content: content, source: source } if matches_filter?(content)
- end
+ save_entry(entries, current_timestamp, current_lines, source) if current_timestamp
entries
rescue StandardError => e
warn "Error parsing #{path}: #{e.message}"
[]
end
+
+ def save_entry(entries, timestamp, lines, source)
+ return if lines.empty?
+
+ content = lines.join
+ entries << { time: timestamp, content: content, source: source } if matches_filter?(content)
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def parse_log_file(path, source) | |
| entries = [] | |
| return entries unless File.exist?(path) | |
| current_timestamp = nil | |
| current_lines = [] | |
| File.readlines(path).each do |line| | |
| if line =~ TIMESTAMP_REGEX | |
| # Save previous entry if exists | |
| if current_timestamp && !current_lines.empty? | |
| content = current_lines.join | |
| entries << { time: current_timestamp, content: content, source: source } if matches_filter?(content) | |
| end | |
| # Start new entry | |
| current_timestamp = Time.parse($1) | |
| current_lines = [$2] | |
| elsif current_timestamp | |
| # Continuation of previous entry | |
| current_lines << line | |
| end | |
| end | |
| # Add final entry | |
| if current_timestamp && !current_lines.empty? | |
| content = current_lines.join | |
| entries << { time: current_timestamp, content: content, source: source } if matches_filter?(content) | |
| end | |
| entries | |
| rescue StandardError => e | |
| warn "Error parsing #{path}: #{e.message}" | |
| [] | |
| end | |
| def parse_log_file(path, source) | |
| entries = [] | |
| return entries unless File.exist?(path) | |
| current_timestamp = nil | |
| current_lines = [] | |
| File.readlines(path).each do |line| | |
| if line =~ TIMESTAMP_REGEX | |
| save_entry(entries, current_timestamp, current_lines, source) if current_timestamp | |
| # Start new entry | |
| current_timestamp = Time.parse(Regexp.last_match(1)) | |
| current_lines = [Regexp.last_match(2)] | |
| elsif current_timestamp | |
| # Continuation of previous entry | |
| current_lines << line | |
| end | |
| end | |
| save_entry(entries, current_timestamp, current_lines, source) if current_timestamp | |
| entries | |
| rescue StandardError => e | |
| warn "Error parsing #{path}: #{e.message}" | |
| [] | |
| end | |
| def save_entry(entries, timestamp, lines, source) | |
| return if lines.empty? | |
| content = lines.join | |
| entries << { time: timestamp, content: content, source: source } if matches_filter?(content) | |
| end |
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] 36-36: Metrics/CyclomaticComplexity: Cyclomatic complexity for parse_log_file is too high. [12/7]
[warning] 36-36: Metrics/PerceivedComplexity: Perceived complexity for parse_log_file is too high. [13/10]
[warning] 52-52: Style/PerlBackrefs: Prefer ::Regexp.last_match(1) over $1.
[warning] 53-53: Style/PerlBackrefs: Prefer ::Regexp.last_match(2) over $2.
🤖 Prompt for AI Agents
In bin/view-synchronized-logs around lines 36 to 70, parse_log_file is overly
complex and uses Perl-style backrefs ($1/$2); refactor by extracting small
helpers (e.g., parse_line_match(line) to return timestamp and body,
flush_current_entry(entries, current_timestamp, current_lines, source) to handle
pushing filtered entries) to reduce cyclomatic complexity and move
parsing/append logic out of the main loop, replace $1/$2 with Regexp.last_match
(or use named captures) to obtain the timestamp and message, and ensure early
returns and single-responsibility helpers so the main method becomes a simple
loop delegating matching, appending, and error handling.
Code Review for PR #1992SummaryThis PR removes the conditional logic that selected different HTTP connection pools based on whether the request is a streaming request or not. Previously, streaming requests used Change: Line 102 now always uses AnalysisContext from Code CommentsThe existing comments at lines 87-89 and 100-101 explain the architectural decision:
The Change# Before:
conn = post_options[:stream] ? connection_without_retries : connection
# After:
conn = connectionCritical Issues🚨 1. Contradicts Documented Architecture (Lines 87-102)The code comments explicitly state:
Yet this PR now uses the retry-enabled connection for all requests, including streaming ones. This appears to reintroduce the issue fixed in #1895 (mentioned in line 245). 🚨 2. Potential Body Duplication in StreamsWhen HTTPX retries a streaming request with the retry plugin enabled, it may send duplicate chunks to the client if the connection drops mid-stream. This is precisely what the two-pool architecture was designed to prevent. From line 78-80 in # Retry logic for streaming requests is handled here by starting fresh requests.
# The HTTPx connection used for streaming has retries disabled (see Request#connection_without_retries)
# to prevent body duplication when partial chunks are already sent to the client.This comment in StreamRequest confirms that streaming requests should not use the retry-enabled connection. 🚨 3. Dead CodeIf
Questions for Author
Potential ImpactHigh Risk Areas:
Lower Risk:
RecommendationsBefore Merging:
Code Quality:
Security & Performance
Verdict: ⛔ Changes Requested This appears to be a significant architectural change that contradicts documented behavior and existing comments. Without clear explanation of intent and thorough testing of streaming scenarios, this risks reintroducing the body duplication bug that was previously fixed. Please provide context for this change and update the PR description, title, and documentation accordingly. Review conducted per repository CLAUDE.md guidelines. Questions? Let me know! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/lib/react_on_rails_pro/request.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
| # For streaming requests, use connection without retries to prevent body duplication | ||
| # The StreamRequest class handles retries properly by starting fresh requests | ||
| conn = post_options[:stream] ? connection_without_retries : connection | ||
| conn = connection |
There was a problem hiding this comment.
Restore non-retrying connection for streaming.
Switching streaming requests to the retry-enabled pool reintroduces the body duplication bug called out just above (Lines 100-101, plus the note referencing issue #1895). We still need connection_without_retries when post_options[:stream] is true; otherwise retries can replay partial stream bodies and break clients. Please keep the conditional selection so streaming uses the non-retry pool.
- conn = connection
+ conn = post_options[:stream] ? connection_without_retries : connection📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn = connection | |
| conn = post_options[:stream] ? connection_without_retries : connection |
🤖 Prompt for AI Agents
In react_on_rails_pro/lib/react_on_rails_pro/request.rb around line 102, restore
the conditional so streaming requests use the non-retrying connection: set conn
= connection_without_retries when post_options[:stream] is true, otherwise set
conn = connection; ensure the conditional branch covers only the stream case and
leaves non-stream requests using the retry-enabled pool.
Code Review for PR #1992Critical Issue: PR Title and Description MismatchThe PR title says "Increase delayBetweenIndividualWorkerRestarts" but the actual code change is completely unrelated. This PR modifies HTTP connection retry behavior for streaming requests, not worker restart delays. What This PR Actually DoesFile changed: Change: Line 102 now uses # Before:
conn = post_options[:stream] ? connection_without_retries : connection
# After:
conn = connection🚨 Major Concerns1. This appears to reintroduce the body duplication bug from #1895Issue #1895 specifically documented that when HTTPX retries a streaming request:
The fix for #1895 was to use a separate connection without retries for streaming requests. This PR removes that fix. 2. Contradicts existing code commentsLines 87-89 explicitly state:
Lines 100-101:
Your change makes these comments incorrect. 3. StreamRequest design relies on no-retry connection
The 4. Dead code implicationsIf this change is intentional, the following code becomes dead:
Questions for Author
RecommendationsBefore Merging:🛑 Do not merge this PR without:
Potential AlternativeIf the goal is to simplify the architecture, consider whether:
Impact AssessmentHigh Risk Areas:
Test Coverage:
Code Quality:
Verdict: ⛔ Changes RequestedThis appears to be a significant architectural change that:
Please do not merge until the above concerns are addressed. Review conducted per repository CLAUDE.md guidelines. Based on codebase analysis and issue #1895 investigation. |
Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit